add task solution #4940
add task solution #4940
Conversation
Welbrn
commented
Oct 15, 2024
Welbrn
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Great job so far! However, there are a few issues that need to be addressed before approval. Please correct the typo in the class name and consider the recommendations on using fixed container size, consistent margins, tag name for styling, and the '*' selector. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| @@ -12,7 +15,50 @@ | |||
| href="./style.css" | |||
| /> | |||
| </head> | |||
| <body> | |||
| <h1>Stars</h1> | |||
| <body class="page__budy"> | |||
There was a problem hiding this comment.
There is a typo in the class name. It should be page__body instead of page__budy. Class names represent the meaning of the content (not the styles or tag names).
| .page__budy { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
Avoid using the '' selector as it impacts performance. It's better to set styles only for elements that require them. Zeroing out your margins, paddings or other styles with '' is still inefficient for the browser.
| width: 16px; | ||
| height: 16px; | ||
| background-size: cover; | ||
| background-position: center; |
There was a problem hiding this comment.
You are using a tag name for styling, which is not recommended. It's better to style all elements using .classes and if needed with :pseudo-classes, pseudo-elements and [attributes].
src/style.css
Outdated
| } | ||
|
|
||
| .stars__star:not(:first-child) { | ||
| margin-left: 4px; |
There was a problem hiding this comment.
Be consistent with your margins. It's better to add only top or bottom margin, but not both, to avoid potential margin collapse.
| .stars--4 .stars__star:nth-child(-n + 4), | ||
| .stars--5 .stars__star:nth-child(-n + 5) { | ||
| background-image: url(./images/star-active.svg); | ||
| } |
There was a problem hiding this comment.
You are using a fixed container size, which is not recommended unless there is a specific requirement. It's better to let the content size dictate it to avoid overflow or accidental scroll bar.
There was a problem hiding this comment.
Great job on your code! There are a few minor issues to address, such as avoiding the use of the * selector for performance reasons and ensuring correct indentation and spacing in your HTML. Also, don't forget to add fallback fonts in your CSS for better compatibility. Keep learning and improving! 💪📚🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| <span class="stars__star"></span> | ||
| <span class="stars__star"></span> | ||
| </div> | ||
| <div class="stars stars--1"> |
There was a problem hiding this comment.
Remember about correct indentation between parent and child elements. Each level of nesting, including text, contained inside the element, requires 2-space offset. Also, blank lines shouldn't be between parent and child elements.
src/index.html
Outdated
| <span class="stars__star"></span> | ||
| <span class="stars__star"></span> | ||
| </div> | ||
| <div class="stars stars--2"> |
There was a problem hiding this comment.
Remember to add empty lines between multiline sibling blocks of HTML to add some 'air' and simplify reading. But don't add them between parent and child elements.
| /* add styles here */ | ||
| .page__body { | ||
| margin: 0; | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
Don't use '' selector (it impacts performance). Set styles only for elements that require them. Zeroing out your margins, paddings or other styles with '' is still inefficient for browser.
